-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add -l option to %R magic to allow passing in of local namespace #2130
Conversation
Rather than adding a separate option, why not make P.S. It looks like your editor is deleting trailing spaces. If you want to make a pull request, can you disable that, so that all the extra changes don't clutter up the diff? Thanks. |
Ah, I see that you did those things. Github oddity: we get notified of comments, but not of new commits on a pull request, so remember to say something when you update it ;-) I think this should also have a test - look in |
OK, I amended the commit -- and also added a parallel change to the %octave magic, which shares the same -i syntax. On Jul 16, 2012, at 10:03 AM, Thomas Kluyver reply@reply.github.com wrote:
|
Test results for commit 1dea0ba merged into master
Not available for testing: python2.6 |
Looks pretty good to me, but I don't use R regularly enough to test this. Outside of the scope of this pull request, I must comment that the |
I think the decorator is a case of explicit is better than implicit |
Yes, I don't think we should parse the function signature, just that the namespace should be passed in the kwargs instead of the args, making usage a simpler:
But this is just bike shedding and not related to this issue. |
Ah, that makes sense, then. Yes, that's probably a good idea. |
I modified the needs_local_scope decorator so that it passes in a local_ns kwarg instead. This makes for a cleaner interface. I tested it with R, but I don't use octave so I couldn't run those tests. |
if args.input: | ||
for input in ','.join(args.input).split(','): | ||
input = unicode_to_str(input) | ||
self._oct.put(input, self.shell.user_ns[input]) | ||
self._oct.put(input, local_ns.get(input, self.shell.user_ns.get(input, None))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a change in behaviour here if the user specifies an input variable that doesn't exist - before it would fail with a KeyError, now it will use None (or the octave equivalent) instead. I think failing is right, because it probably means the user mistyped something. It might take a couple of extra lines to get it to fail properly, though.
That makes sense. I just switched it from user_ns.get() to back to user_ns[] will cause the same KeyError as before. |
Except I think that will throw the error if you pass a variable that only exists in the local namespace, because the second argument to
(Python 3.3 has a handy ChainMap class that will make this sort of thing easier, but for now we have to do it the long way.) |
Thanks. I fixed that and added some doctests to cover that case. |
...: pass | ||
...: | ||
|
||
In [9]: isinstance(e, KeyError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will fail in Python 3 - e
only exists within the except clause, so it's a NameError
to refer to it here. This is the sort of thing I don't like about doctests - they're quite brittle.
Ideally, we could convert this into a standard unit test - use ip.run_cell
to define the function, ip.run_line_magic
to call %R
and nt.assert_raises
to check that this bit throws a KeyError.
Pinging @guyhf. Just a few more quick changes here and I think this is ready to be committed. |
Just updating it now. I'll convert the KeyError exception test to a standard unit test. |
I changed the KeyError test to a unit test and tested both IPython.extensions.tests.test_rmagic and IPython.extensions.tests.test_octavemagic. I'm fine changing the other doctests to unit tests if that is what is preferred. |
That looks fine. I prefer unit tests because I've often had to fight with both doctests and the machinery that runs them (if you ever want a free headache, look at @bfroehle: I'll let you have another look at this, but if you're happy, you can merge it. |
I went ahead and changed the rest of the doctests to unit tests and re-checked IPython.extensions.tests.test_rmagic and IPython.extensions.tests.test_octavemagic. |
result = ip.user_ns['result'] | ||
np.testing.assert_equal(result, 1) | ||
|
||
ip.run_cell('''def test_octavemagic_localscope(u): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call this function something different from the test, to avoid confusion. Also, while we're at it, let's not have 'test' in the name, just in case nose somehow finds it (it attempts to run any function it finds named 'test').
Test results for commit 5684165 merged into master (3c7c235)
Not available for testing: |
With those small changes that @takluyver suggests in the tests, I think it's ready to go. |
I renamed the internal test functions. |
try: | ||
val = local_ns[input] | ||
except KeyError: | ||
val = self.shell.user_ns[input] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just occurred to me that this is more-or-less equivalent to eval(input, self.shell.user_ns, local_ns)
(with the exception that it'll raise a NameError instead of a KeyError if the key cannot be found).
But I think I like the extended try/except block better. Certainly it's faster to execute.
|
||
def test_octavemagic_localscope(): | ||
ip = get_ipython() | ||
ip.magic('load_ext octavemagic') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make any other changes this should probably be changed to ip.run_line_magic('load_ext', 'octavemagic')
, but there are enough other instances of ip.magic
in the test routines where it's clearly not actually an issue.
Great. I think this could be merged now. :) |
…n local scope before looking in user_ns. This allows the magics to be called from within functions. Also Modified @needs_local_scope decorator for magic extensions to use a kwarg with the local namespace, so functions now get a separate kward local_ns, e.g. def magic_function(line, cell=None, local_ns=None): ...
Switched np.testing.assert_equals to nt.assert_equals. |
Search local namespaces for input variables in %R and %octave magics.
@guyhf Merged! Thanks for all your work and all your patience. I think we ended up with an excellent result. |
Search local namespaces for input variables in %R and %octave magics.
Feature request for
rmagic
: It would be great to be able to pass in local variables to the%R
magic so that you can write functions that invoke R with values from the local scope (not just from user_ns). For example:Right now with
%R -i
magic you can only get variables from user_ns, so the above code won't work. I have added a%R -l
argument that works just like%R -i
except that variables are taken from the local scope. So the above code would be written:And the result would be:
The following changes to rmagic.py would make this work.
guyhf@5448c93